Normalise metric names prior to checking against filterlist - #55599
Normalise metric names prior to checking against filterlist#55599StephenWakely wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Patch is incorrect. Prefix filter entries ending in normalizable separators are broadened unexpectedly, and the new package documentation links to a nonexistent file.
| // This is a faithful port of `NormMetricNameParse` / `ValidateMetricName` in | ||
| // dd-go (`model/metric.go`). Keep the two in sync: a divergence here silently | ||
| // changes which metrics get filtered. See `normalisation.md` at the repository |
There was a problem hiding this comment.
normalisation.md does not exist at the repository root, leaving the package's promised rule/provenance documentation as a broken reference. Add the document or remove/update this link.
Go Package Import DifferencesBaseline: ad97d80
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: 802f91b | Docs | View more details | Give us feedback! |
Files inventory check summaryFile checks results against ancestor ad97d80a: Results for datadog-agent_7.84.0~devel.git.576.802f91b.pipeline.134134316-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.576.802f91b.pipeline.134134316-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
24 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: ad97d80 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_metric_filterlist_10k_rewrite | memory utilization | +0.23 | [+0.01, +0.44] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.09 | [+0.04, +0.13] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.03 | [-0.01, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.06 | [-0.09, -0.02] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.11 | [-0.19, -0.02] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.27 | [-0.39, -0.16] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | -0.34 | [-0.56, -0.13] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.38 | [-0.42, -0.35] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metric_filterlist_10k | memory utilization | -0.82 | [-1.04, -0.59] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.83 | [-1.06, -0.59] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | -1.06 | [-1.30, -0.82] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -1.51 | [-2.37, -0.64] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 173.87MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 746.62KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 513.52MiB ≤ 537MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 209.69MiB ≤ 220MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.37MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metric_filterlist_10k | cpu_usage | 10/10 | 237.53 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metric_filterlist_10k | memory_usage | 10/10 | 0.37GiB ≤ 1.50GiB | bounds checks dashboard |
| ✅ | quality_gate_metric_filterlist_10k_rewrite | cpu_usage | 10/10 | 236.98 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metric_filterlist_10k_rewrite | memory_usage | 10/10 | 0.36GiB ≤ 1.50GiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 388.15 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 21 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 434.89MiB ≤ 455MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 72.33MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 30.28 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 323.37MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 62.85 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 303.16MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 22.84 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 310.37MiB ≤ 345MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metric_filterlist_10k_rewrite, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metric_filterlist_10k_rewrite, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metric_filterlist_10k, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metric_filterlist_10k, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
The absolute 10us median bound failed under CI's 'bazel coverage --config=gorace': race plus coverage instrumentation inflates an ~80ns lookup to ~21us (~260x), so no absolute bound can be both survivable there and meaningful uninstrumented. Assert the ratio of median lookup at 100k entries to 10k instead. That is invariant to instrumentation while still catching a regression to a linear scan, whose O(n) term grows past the fixed per-call overhead either way. Measured: 1.04x plain, 1.03x under race+coverage, ~10x if linear. Also cut samples 200k -> 20k, which keeps percentiles stable and drops the test from 21.7s to ~2s under the race detector.
All 7 copied filterlist tiers crashed every replicate with a non-zero exit, while all 8 pre-existing cases in the same job passed 10/10. Uniform failure across tiers ruled out filterlist size; the differences from the comparable working case (quality_gate_metrics_logs) were the target resources and the offered load, neither of which had ever been validated in a real SMP job. Drop 20k-100k and bring 10k in line with quality_gate_metrics_logs: cpu_allotment 6 -> 4 memory_allotment 8192 MiB -> 2GiB offered load 60 MiB/s -> 20 MiB/s (18/1/1, 90/5/5 split preserved) The 8192 MiB allotment was never measured; it had been raised 1024 -> 2048 -> 4096 -> 8192 reactively in response to these same crashes. Bounds are now ceilings below the allotment rather than equal to it, so memory_usage can actually fail; tighten both once a run reports a peak. Verified the traffic still exercises the filter list: all 676 guaranteed-match names are in the 10k list, no guaranteed-nomatch name is, patterns are character ranges so lading cannot rewrite the padding.
Test allocated when the submitted name needed rewriting, because Normalize
has to return a string. That is not a rare path in practice: lading's
DogStatsD generator draws names from letters and digits only, and 10 of
those 62 characters are digits, so ~16% of generated names do not start
with a letter and are therefore not normalized. Measured over lading-shaped
names that came to 34 B/op amortised, ~630 KB/s of garbage at the 18 MiB/s
the paired SMP case offers, which is the likeliest source of the small
memory delta that case reported.
Split the parser out as normalizeAppend so Test can build the normalized
form in a stack buffer. A normalized name is never longer than its input and
firstAlpha rejects anything over MaxLength, so append never reallocates and
the buffer does not escape.
Measured on lading-shaped names against a 10k list, same seed and matcher:
before after
mixed traffic 566 ns 34 B/op 512 ns 0 B/op
normalized only - 503 ns 0 B/op
needs rewrite - 566 ns 0 B/op
Roughly 10% faster on the mix rather than slower: two allocations cost more
than zeroing a 350-byte stack buffer. The fast path is unchanged, and the
mix matches 0.84*503 + 0.16*566 = 513 as expected.
…iting quality_gate_metric_filterlist_10k barely exercises the normalisation rewrite path. Its background generator is 90% of the traffic and uses lading's random names, whose alphabet is letters and digits only; a normalised name must start with a letter, and 10 of those 62 characters are digits, so only ~16% of names need rewriting. Add quality_gate_metric_filterlist_10k_rewrite, identical to the base case in every other respect (10k entries, 3 generators at 18/1/1 MiB/s, 4 CPU, 2GiB), where every name carries '-' characters and so must be rewritten. The filter list holds the normalised spelling, so this also fails loudly if normalisation is ever removed from Matcher.Test: nothing would be filtered. Read this case for CPU, not memory. All three generators need explicit metric_names pools because lading's random alphabet cannot emit '-', so background contributes 13520 distinct names instead of the base case's contexts: 1000..10000, which moves the aggregator's context count for reasons unrelated to what is being measured. Noted in the case README. Also restructure the Go path benchmarks as BenchmarkMatcherTestPaths, holding probe count, name length and list shape constant across variants. The previous set was confounded: differing fixture sizes and layout made an unrelated cache effect look like a code-path difference, reporting the rewrite path as 4x FASTER than the fast path. With shape held constant: already-normalized 146-170 ns/op 0 allocs hyphenated 180-199 ns/op 0 allocs <- the new SMP case's shape leading-digit 187-195 ns/op 0 allocs trailing-hyphen 251-259 ns/op 0 allocs <- pathological for the check Each variant asserts its own premise before measuring, which is what caught a fixture bug here: '-' normalises to '_', not '.', so the entries have to be the underscore spelling.
What does this PR do?
Motivation
Describe how you validated your changes
Additional Notes